Skip to content

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Apr 3, 2020

I don't quite buy the justification in https://rust-lang.github.io/rust-clippy/. The justification is:

It makes the code less readable.

In the Rust codebases I've worked in, I have found people were comfortable using match bool (selectively) to make code more readable. For example, initializing struct fields is a place where the indentation of match can work better than the indentation of if:

let _ = Struct {
    v: {
        ...
    },
    w: match doing_w {
        true => ...,
        false => ...,
    },
    x: Nested {
        c: ...,
        b: ...,
        a: ...,
    },
    y: if doing_y {
        ...
    } else { // :(
        ...
    },
    z: ...,
};

Or sometimes people prefer something a bit less pithy than if when the meaning of the bool doesn't read off clearly from the condition:

if set.insert(...) {
    ... // ???
} else {
    ...
}

match set.insert(...) {
    // set.insert returns false if already present
    false => ...,
    true => ...,
}

Or match can be a better fit when the bool is playing the role more of a value than a branch condition:

impl ErrorCodes {
    pub fn from(b: bool) -> Self {
        match b {
            true => ErrorCodes::Yes,
            false => ErrorCodes::No,
        }
    }
}

And then there's plain old it's-1-line-shorter, which means we get 25% more content on a screen when stacking a sequence of conditions:

let old_noun = match old_binding.is_import() {
    true => "import",
    false => "definition",
};
let new_participle = match new_binding.is_import() {
    true => "imported",
    false => "defined",
};

Bottom line is I think this lint fits the bill better as a pedantic lint; I don't think linting on this by default is justified.

changelog: Remove match_bool from default set of enabled lints

@dtolnay
Copy link
Member Author

dtolnay commented Apr 3, 2020

Build failure appears unrelated.

@flip1995
Copy link
Member

flip1995 commented Apr 4, 2020

I think the reasoning

It makes the code less readable.

is a little too weak for this lint.


In your first code example I'd prefer the if over the match, since I don't think that it is less readable. If the code is short, this can be formatted as a one-liner if x { a } else { b }. If the if/else is longer it should be moved into a binding and the binding should be used to initialize the struct.

For your second example, you could still put the comment on top of the if-block. If it is not about the comment you could also write if set.insert() == false, which doesn't clarify the intent though and is bad style IMO.

The enum-from-bool example is a special case, where you could allow this lint once in the code base. After that you probably only use the enum and don't match on the bool anymore.

I can see the "it's shorter" argument.


Overall, this lint is opinionated (which is fair for style group lints), so marking it as C-needs-discussion. cc @rust-lang/clippy

@flip1995 flip1995 added the S-needs-discussion Status: Needs further discussion before merging or work can be started label Apr 4, 2020
@djc
Copy link
Contributor

djc commented Apr 10, 2020

FWIW, I'm also not a fan of this lint, for similar reasons as @dtolnay.

@tesuji
Copy link
Contributor

tesuji commented Apr 23, 2020

I'm for one to downgrade this lint.

@flip1995 flip1995 added S-waiting-on-bors Status: The marked PR was approved and is only waiting bors and removed S-needs-discussion Status: Needs further discussion before merging or work can be started labels Apr 23, 2020
@flip1995
Copy link
Member

@dtolnay This needs a rebase. After that, we can merge this.

@dtolnay
Copy link
Member Author

dtolnay commented Apr 23, 2020

Rebased.

@flip1995
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 25, 2020

📌 Commit ef28361 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Apr 25, 2020

🌲 The tree is currently closed for pull requests below priority 1, this pull request will be tested once the tree is reopened

flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Apr 25, 2020
Downgrade match_bool to pedantic

I don't quite buy the justification in https://rust-lang.github.io/rust-clippy/. The justification is:

> It makes the code less readable.

In the Rust codebases I've worked in, I have found people were comfortable using `match bool` (selectively) to make code more readable. For example, initializing struct fields is a place where the indentation of `match` can work better than the indentation of `if`:

```rust
let _ = Struct {
    v: {
        ...
    },
    w: match doing_w {
        true => ...,
        false => ...,
    },
    x: Nested {
        c: ...,
        b: ...,
        a: ...,
    },
    y: if doing_y {
        ...
    } else { // :(
        ...
    },
    z: ...,
};
```

Or sometimes people prefer something a bit less pithy than `if` when the meaning of the bool doesn't read off clearly from the condition:

```rust
if set.insert(...) {
    ... // ???
} else {
    ...
}

match set.insert(...) {
    // set.insert returns false if already present
    false => ...,
    true => ...,
}
```

Or `match` can be a better fit when the bool is playing the role more of a value than a branch condition:

```rust
impl ErrorCodes {
    pub fn from(b: bool) -> Self {
        match b {
            true => ErrorCodes::Yes,
            false => ErrorCodes::No,
        }
    }
}
```

And then there's plain old it's-1-line-shorter, which means we get 25% more content on a screen when stacking a sequence of conditions:

```rust
let old_noun = match old_binding.is_import() {
    true => "import",
    false => "definition",
};
let new_participle = match new_binding.is_import() {
    true => "imported",
    false => "defined",
};
```

Bottom line is I think this lint fits the bill better as a pedantic lint; I don't think linting on this by default is justified.

changelog: Remove match_bool from default set of enabled lints
This was referenced Apr 25, 2020
bors added a commit that referenced this pull request Apr 25, 2020
Rollup of 5 pull requests

Successful merges:

 - #5408 (Downgrade match_bool to pedantic)
 - #5505 (Avoid running cargo+internal lints when not enabled)
 - #5516 (Add a note to the beta sections of release.md)
 - #5517 (Deploy time travel)
 - #5523 (Add lifetime test case for `new_ret_no_self`)

Failed merges:

r? @ghost

changelog: rollup
@bors bors merged commit e1d13c3 into rust-lang:master Apr 25, 2020
@dtolnay dtolnay deleted the matchbool branch April 27, 2020 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: The marked PR was approved and is only waiting bors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants